-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JavaEE to Quarkus 2 Declarative Migration Recipe #73
Conversation
Great start @ShatanikB ! Good to hear these recipes already work for you on real applications. My suggestion for maintainability going forward is that we move your declarative recipes into a separate I've also added a test just now because that triggers some of the validations we do through our test framework. I'll work on getting the unit tests to pass on |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/test/java/org/openrewrite/quarkus/quarkus2/JavaEEtoQuarkus2CodeTranformationsTest.java
Outdated
Show resolved
Hide resolved
…2CodeTranformationsTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks both! From what I'm seeing this is ready for review and merge.
Just a note that I will have a look this week. |
Sorry for the newbie mistakes. For some reason, it shows that @ammachado is committing changes instead of me. |
Those commits are quite confusing indeed; could you check both your user home |
…s Maven only right now. Added a TODO for Gradle
Thanks again. My user was not configured at all on codespaces. Fixed it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, time is scarce on my side these days.
I added some comments but it looks good overall.
- org.openrewrite.java.dependencies.AddDependency: | ||
groupId: io.quarkus | ||
artifactId: quarkus-undertow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I would add this one except if the application is explicitly using servlet.
Especially since having undertow around changes the behavior of Quarkus.
Now this can probably be done as a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added undertow to be able to use the 'SessionScoped' annotation as per this . Making the assumption that @javax.ejb.Stateful
should be mapped to @javax.enterprise.context.SessionScoped
in CDI
- org.openrewrite.java.dependencies.AddDependency: | ||
groupId: io.quarkus | ||
artifactId: quarkus-hibernate-orm | ||
- org.openrewrite.java.dependencies.AddDependency: | ||
groupId: io.quarkus | ||
artifactId: quarkus-jdbc-h2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, I don't think I would add these two except if the application makes use of persistence.
Now this can probably be done as a follow-up.
...st/java/org/openrewrite/quarkus/quarkus2/JavaEEtoQuarkus2MavenDependenciesMigrationTest.java
Outdated
Show resolved
Hide resolved
displayName: Migrate JavaEE Maven Dependencies to Quarkus 2 | ||
description: Upgrade Standard JavaEE dependencies to Quarkus 2 dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a recipe locally that does exactly this (and most part of the dependency handling of this recipe). It adds the Camel Extensions for Quarkus, and the output is closer to what quarkus init
generates. I can contribute it on a separate PR (but I need openrewrite/rewrite#4006 merged before that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#4006 is now merged if you'd like to open that other PR @ammachado
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ammachado Did you have a PR for similar functionality ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work here @ShatanikB ! I've refreshed your PR with a couple of new insights since it was opened, and added small steps like removing the maven-war-plugin. Hope you agree,
It took longer than I'd wanted to see this through, as I had left space for additional review. But enough time has passed now that we'll just continue to work on the main branch if anything new feedback arrives, since you've been patient enough to get this merged.
Thanks to all involved; hope this helps drive adoption towards Quarkus from Java EE 7, and from there onwards to Quarkus 3+.
* JavaEE to Quarkus 2 Declarative Migration Recipe * Add JavaEEtoQuarkus2MigrationTest and verify pom changes * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Split Recipe to Maven Dependencies Upgrade YML and Code Tranformations YML * Updated Tests * Remove recipe from quarkus.yml * Merge all three JavaEEtoQuarkus2 recipes into one file * Update src/test/java/org/openrewrite/quarkus/quarkus2/JavaEEtoQuarkus2CodeTranformationsTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Updated Dependency Migration Description to reflect that this migrates Maven only right now. Added a TODO for Gradle * Updated dependency versions and tests * Move recipes and reformat tests * Break up dependency & maven recipes; expect to remove war plugin * Bump failsafe and surefire plugin versions --------- Co-authored-by: Adriano Machado <[email protected]> Co-authored-by: Tim te Beek <[email protected]> Co-authored-by: Tim te Beek <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
What's changed?
Added a declarative recipe to migrate Java EE to Quarkus 2. Started with adding Quarkus BOM, Quarkus extension dependencies, removed JEE dependencies, converted some EJB annotations to CDI (Stateful, Stateless and Singleton), changed JPA annotations to work with Quarkus
What's your motivation?
Contributing to #72
Anything in particular you'd like reviewers to focus on?
General feedback on Best practices I might be missing
Anyone you would like to review specifically?
@timtebeek
Any additional context
Trying to keep it declarative. Will add imperative recipes if required.
Checklist
[ ] I've added unit tests to cover both positive and negative casesNo because composite recipe. Have tested on real applications